Skip to content

Conversation

Josh194
Copy link
Contributor

@Josh194 Josh194 commented May 15, 2025

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Previously, attempting to use an overridden config containing a plugin with a circular structure (as is suggested for flat configs) would error, as the caching implementation would attempt to JSON.stringify said plugins in order to use them as cache keys.

This PR fixes the issue by using flatted to perform the conversion instead, which is capable of handling circular objects.

A test case (verified to error without the fix) is also included.

Fixes: #270.

Breaking Changes

None.

Additional Info

The changes to jsonStringifyReplacerSortKeys are required as returning new objects from the replacer function causes flatted to enter an infinite loop as it attempts to deal with the ever expanding set of references, eventually running out of memory. I address that by overwriting the object in-place; I'd strongly recommend giving those specific changes a good look, as any mistakes could corrupt the options object for all later code. In particular, I'm not 100% sure if Object.assign is the best option here, but it could be completely fine.

Test case plugin code is taken almost exactly from https://eslint.org/docs/latest/extend/plugin-migration-flat-config#migrating-configs-for-flat-config; it is probably possible to make it a bit shorter, but I didn't bother to look into it.

npm run test was run immediately prior to the final (and only) commit, so any stylistic lints should have been applied, but apologies if I've missed anything.

Replaces `JSON.stringify` with an implementation from `flatted` for cache keys.
@Josh194
Copy link
Contributor Author

Josh194 commented May 15, 2025

Test failures seem to be exclusive to Eslint 8.x builds (probably to do with the test plugin I create in circular-plugin.test.js). Not entirely sure what that might be right now. I can't change much about loaderOptions, since all three options are required to run properly. If it's something to do with the actual plugin variable itself, it'll probably just be a question of trimming some of the fields. I'll see if I can get an 8.x test running locally.

@Josh194
Copy link
Contributor Author

Josh194 commented May 15, 2025

After some investigation, this just seems to be a result of eslint-webpack-plugin not allowing use of flat configs on ESLint 8.x (at least I think; specific error is coming from worker.js:52). Removing just the new test locally makes all others succeed again. I briefly looked at potentially addressing the flat config issue directly, but seems to be far out of scope for this PR.

Would you be alright with me just restricting the test case to only ESLint ^9.x (using the same design as flat-config.test.js)? The changes here should be completely back-compatible, so old ESLint versions should still work fine, they just don't get the circular plugin support.

If so, please let me know how you'd like me to provide that change (additional commit? new PR? etc).

@alexander-akait
Copy link
Member

Would you be alright with me just restricting the test case to only ESLint ^9.x (using the same design as flat-config.test.js)? The changes here should be completely back-compatible, so old ESLint versions should still work fine, they just don't get the circular plugin support.

I am fine with it

@Josh194
Copy link
Contributor Author

Josh194 commented May 15, 2025

Done. I've tested the new version locally on ESLint 8.x based on the github workflows; hopefully my setup is close enough to the actual workflows that it runs properly now.

@alexander-akait
Copy link
Member

/cc @ricardogobbosouza can you look at this?

Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3adbe4c) to head (27f440d).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #277   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          291       307   +16     
  Branches        81        84    +3     
=========================================
+ Hits           291       307   +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexander-akait
Copy link
Member

@Josh194 ping me after 3 days if we don't get feedback from @ricardogobbosouza, thank you

@ricardogobbosouza
Copy link
Collaborator

Thanks @Josh194

@ricardogobbosouza ricardogobbosouza merged commit 32f05c6 into webpack-contrib:master May 16, 2025
29 checks passed
@Josh194 Josh194 deleted the flatten branch May 17, 2025 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Maximum call stack size exceeded" when running with eslnt 9

3 participants